Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: enable the exceptions category for all targets #1082

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jan 7, 2024

Summary

Clean up the category and enable it for all targets by default. This
significantly increases test coverage of exceptions for the JS and VM
targets.

Details

General changes/cleanup:

  • references to issues like #1234 are replaced with proper GitHub
    links
  • the targets key is removed for tests that should be run on all
    targets
  • usage of cmd is replaced with usage of matrix (allowing for
    multi-target testing)
  • write(stdout, ...) is replaced with echo in order to support the
    JS and VM targets
  • execution of static: blocks is replaced with using the VM target,
    when the test is not specific to compile-time execution
  • tests that don't work but should are marked as knownIssue

Specific changes:

  • where not necessary for the test, getCurrentExceptionMsg is
    replaced with getCurrentException().msg, as the former is not yet
    supported by the VM
  • texceptions2.nim is removed -- it's redundant with texception.nim
  • texception_message_null_byte.nim is turned into a normal test that
    doesn't rely on self-execution

Summary
=======

Clean up the category and enable it for all targets by default. This
significantly increases test coverage of exceptions for the JS and VM
targets.

Details
=======

General changes/cleanup:
* references to issues like `nim-works#1234` are replaced with proper GitHub
  links
* the `targets` key is removed for tests that should be run on all
  targets
* usage of `cmd` is replaced with usage of `matrix` (allowing for
  multi-target testing)
* `write(stdout, ...)` is replaced with `echo` in order to support the
  JS and VM targets
* execution of `static:` blocks is replaced with using the VM target,
  when the test is not specific to compile-time execution
* tests that don't work but should are marked as `knownIssue`

Specific changes:
* where not necessary for the test, `getCurrentExceptionMsg` is
  replaced with `getCurrentException().msg`, as the former is not yet
  supported by the VM
* `texceptions2.nim` is removed -- it's redundant with `texception.nim`
* `texception_message_null_byte.nim` is turned into a normal test that
  doesn't rely on self-execution
@zerbina zerbina added the test Add or improve tests label Jan 7, 2024
@zerbina
Copy link
Collaborator Author

zerbina commented Jan 7, 2024

The texception_message_null_byte.nim test works for me locally (Windows), but fails in the CI. It's probably an issue with the null byte and testament's output comparison -- I'll investigate.

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change itself looks good, testament issue aside.

@saem
Copy link
Collaborator

saem commented Jan 10, 2024

/merge

Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Jan 10, 2024
Merged via the queue into nim-works:devel with commit 96a90f3 Jan 10, 2024
18 checks passed
@zerbina zerbina deleted the tests-exception-cleanup branch January 10, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Add or improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants